gui: remove click+wait forever trap in Timing Report #5925
Conversation
In small designs a large number of paths can easily be displayed, but for large designs this effectively runs forever. To avoid this lockup, do not save number of timing paths Signed-off-by: Øyvind Harboe <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
If we are going to accept this, we need to open an issue so we can track fixing this.
It might be worth a testcase so someone can actually take a look at this and fix it.
Possibly a a better solution would be to implement a way to load the gui without loading the settings file (ie. honor -no_init).
src/gui/src/timingWidget.cpp
Outdated
| // NOTE! Don't save path count, this can introduce problems when | ||
| // switching between large and small designs. Clicking Timing Report | ||
| // Update for a large number of paths will freeze the GUI, effectively | ||
| // for large designs. |
There was a problem hiding this comment.
it would be sufficient to just avoid loading the value (but still save it).
There was a problem hiding this comment.
would not loading the settings file be sufficient instead?
There was a problem hiding this comment.
I still want saved settings, I just don't want the settings that can lock up(as in it is faster to kill the GUI and reload) the GUI for large designs.
There was a problem hiding this comment.
I mean something like this: #5929
There was a problem hiding this comment.
Seems useful, but I don't understand the connection to this PR. In this PR, I'm trying to address a specific option that is causing the GUI to "lock up".
There was a problem hiding this comment.
no real connection, except it would also allow you to avoid loading the path_count, while preserving the functionality to save and load it until a solution to the speed is addressed. My personal preference would be to file an issue on the speed thing and see if it can be addressed before changing the default behavior for the GUI.
There was a problem hiding this comment.
I just assumed it would take a considerable amount of time to address so I skipped that step...
Wrong?
There was a problem hiding this comment.
@oharboe without a testcase and a filed issue, then yes, it will take a very long time to get addressed. Please open an issue so it can be addressed at the root of the issue.
There was a problem hiding this comment.
feature request added, this PR is "boarding up the broken window".
until a sufficiently good heuristic can be found or a better way to handle long update times without blocking the GUI Signed-off-by: Øyvind Harboe <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@maliberty Ping? Move ahead with this? |
|
I still don't think this is the right solution. It's a solution that takes away functionality for the perceived benefit of one user with a particular problem (when I ran it on my machine the delay was a few minutes so not too terrible). I'd rather see a warning dialog over removing this. |
|
What would the warning say? your machine is locked up? The user knows that already. |
|
This is not a choice between nice alternatives: pick some misbehavior until it can be fixed properly. |
|
my understanding is that it is a nontrivial amount of work to fix properly(a generic progressbar with a "cancel" capability for long running operations in the GUI), so whatever misbehavior we pick(we have one today), we will be living with it for some time. |
|
@gadfort already created an option to skip loading the settings. Is that sufficient? |
No, this fix is to avoid a suckerpunch in the first place. If I knew to use that option, I wouldn't click "Update" and lock up the GUI in the first place... |
|
Let's see if this is solvable in a nicer way - parallaxsw/OpenSTA#111 |
In small designs a large number of paths can easily be displayed, but for large designs this effectively runs forever.
To avoid this lockup trap, do not save number of timing paths.
This is not ideal, but if the choice is between saving path count and the gui "crashing" for large designs by accidentally clicking "Timing Report Update", then it seems better not to save for now.
Perhaps in the future, for large designs, a progress requester could be made and a "display fetched paths so far" could be added or the performance could be improved to the point that the problem can be ignored.